Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

update json-patch to fix nil value issue when creating mergepatch #49259

Merged
merged 2 commits into from
Jul 26, 2017
Merged

update json-patch to fix nil value issue when creating mergepatch #49259

merged 2 commits into from
Jul 26, 2017

Conversation

dixudx
Copy link
Member

@dixudx dixudx commented Jul 20, 2017 •
edited

What this PR does / why we need it:
When creating a patch for merge, nil value will be considered as different value. This has been fixed and merged in evanphx/json-patch #45.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #49044

Special notes for your reviewer:
/cc @MikeSpreitzer @mengqiy

Release note:

Fix nil value issue when creating json patch for merge

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 20, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @dixudx. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 20, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 20, 2017
@mengqiy
Copy link
Member

mengqiy commented Jul 20, 2017

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 20, 2017
@dixudx dixudx force-pushed the fix_jsonpatch_nil_value_merge branch 2 times, most recently from 45d7b4b to ba3dcea Compare July 20, 2017 07:25
@dixudx
Copy link
Member Author

dixudx commented Jul 20, 2017

@mengqiy Need your approval. Thanks.

@mengqiy
Copy link
Member

mengqiy commented Jul 20, 2017

Want to make sure that all the test files change under pkg/kubectl/cmd/testdata/edit/ are all auto-recorded. You didn't manually change them, right?

@dixudx
Copy link
Member Author

dixudx commented Jul 21, 2017

@mengqiy I manually change them. Also I don't know why ~1 are kept left there.

@dixudx
Copy link
Member Author

dixudx commented Jul 21, 2017

@shiywang I noticed test files under pkg/kubectl/cmd/testdata/edit/ were created in your PR #42256. Does ~1 in kubectl.kubernetes.io~1last-applied-configuration have some special meanings? I manually replace ~1 with /. @mengqiy and I want to evaluate the impact. Thanks.

@dixudx
Copy link
Member Author

dixudx commented Jul 25, 2017

@lavalamp @brendandburns PTAL. Thanks.

shiywang
shiywang reviewed Jul 25, 2017
View reviewed changes
pkg/kubectl/cmd/testdata/edit/testcase-apply-edit-last-applied-syntax-error/3.request
@@ -1,7 +1,7 @@
{
"metadata": {
"annotations": {
"kubectl.kubernetes.io~1last-applied-configuration": "{\"kind\":\"Service\",\"metadata\":{\"annotations\":{},\"labels\":{\"app\":\"svc1\",\"new-label\":\"foo\",\"new-label1\":\"foo1\"},\"name\":\"svc1\",\"namespace\":\"myproject\"},\"spec\":{\"ports\":[{\"name\":\"80\",\"port\":81,\"protocol\":\"TCP\",\"targetPort\":81}],\"sessionAffinity\":\"None\",\"type\":\"ClusterIP\"},\"status\":{\"loadBalancer\":{}}}\n"
"kubectl.kubernetes.io/last-applied-configuration": "{\"kind\":\"Service\",\"metadata\":{\"annotations\":{},\"labels\":{\"app\":\"svc1\",\"new-label\":\"foo\",\"new-label1\":\"foo1\"},\"name\":\"svc1\",\"namespace\":\"myproject\"},\"spec\":{\"ports\":[{\"name\":\"80\",\"port\":81,\"protocol\":\"TCP\",\"targetPort\":81}],\"sessionAffinity\":\"None\",\"type\":\"ClusterIP\"},\"status\":{\"loadBalancer\":{}}}\n"
Copy link
Contributor

@shiywang shiywang Jul 25, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't do any manually editing even it's right, please use kubectl apply ... --v=loglevel to recording request and response, then paste the whole result here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to copy and paste.
Just set UPDATE_EDIT_FIXTURE_DATA to true, the test will auto record the result.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/edit_test.go#L87

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mengqiy It works. Excellent!

@shiywang The files just updated and are exactly the same with my changes.

I think it is safe to get merged.

@dixudx dixudx force-pushed the fix_jsonpatch_nil_value_merge branch from ba3dcea to 2235d8d Compare July 25, 2017 04:57
@dixudx
Copy link
Member Author

dixudx commented Jul 25, 2017

@brendandburns @dchen1107 @lavalamp @jbeda @mengqiy Need your lgtm and approval.

@mengqiy
Copy link
Member

mengqiy commented Jul 25, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2017
@brendandburns
Copy link
Contributor

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, dixudx, mengqiy

Associated issue: 45

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2017
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@dixudx
Copy link
Member Author

dixudx commented Jul 26, 2017

/retest

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jul 26, 2017 •
edited

@dixudx: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-etcd3 2235d8d link /test pull-kubernetes-e2e-gce-etcd3

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 49259, 49350)

@k8s-github-robot k8s-github-robot merged commit 778da50 into kubernetes:master Jul 26, 2017
@dixudx dixudx deleted the fix_jsonpatch_nil_value_merge branch July 26, 2017 03:28
@yue9944882 yue9944882 mentioned this pull request Feb 28, 2018
Merged
k8s-github-robot pushed a commit that referenced this pull request Mar 2, 2018
Merge pull request #59324 from yue9944882/fix-json-encode-slash-escape
e47765d 
Automatic merge from submit-queue.

Automated cherry pick of #49259: update json-patch to fix nil value issue when creating mergepatch

Cherry pick of #49259 on release-1.7.

#49259: update json-patch to fix nil value issue when creating mergepatch
@wgliang wgliang mentioned this pull request May 25, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mengqiy mengqiy mengqiy left review comments

@shiywang shiywang shiywang left review comments

Assignees

@lavalamp lavalamp

@mengqiy mengqiy

@brendandburns brendandburns

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kubectl annotate patches too much
7 participants
@dixudx @k8s-ci-robot @mengqiy @brendandburns @k8s-github-robot @shiywang @lavalamp